Skip to content

feat: migrate to Connect-RPC; new wire path /kv.v2.KvService/#119

Merged
rustatian merged 10 commits into
masterfrom
feature/migrate-connect-rpc
May 11, 2026
Merged

feat: migrate to Connect-RPC; new wire path /kv.v2.KvService/#119
rustatian merged 10 commits into
masterfrom
feature/migrate-connect-rpc

Conversation

@rustatian
Copy link
Copy Markdown
Member

@rustatian rustatian commented May 11, 2026

Summary

  • plugin.go: RPC() anyRPC() (string, http.Handler) returning kvV2connect.NewKvServiceHandler. The rpc plugin's HTTP/2 mux mounts the handler at /kv.v2.KvService/.
  • rpc.go: 7 methods (Has, Set, MGet, MExpire, TTL, Delete, Clear) adapted to the generated Connect handler interface.
  • OTEL spans now use the caller's ctx instead of context.Background(), so client cancellation/deadlines propagate into the storage operations.
  • Empty StorageCodeInvalidArgument; unknown storage → CodeNotFound; storage errors wrap errors.E(op, err) with CodeInternal.
  • A small lookupStorage helper DRYs out the 7 identical "no storage / no such storage" guard blocks.
  • Tests use connectrpc.com/connect with an http2.Transport h2c client at 127.0.0.1:6001; the goridge codec is no longer imported.
  • Deps: api-go v6.0.0-beta.5, rpc/v6 v6.0.0-beta.4 (the Connect-migrated rpc plugin), connectrpc.com/connect v1.19.2, golang.org/x/net for h2c.
  • .golangci.yml: removed dead settings.dupl block (dupl wasn't in the enabled linters list).

Breaking changes

  • Wire protocol changes from goridge net/rpc codec to Connect-RPC over HTTP/2. Old clients (PHP using goridgeRpc) cannot reach the kv plugin once a roadrunner build with this change is deployed.
  • Service is now registered at /kv.v2.KvService/{Method} instead of kv.{Method}.
  • Empty storage and unknown-storage errors previously surfaced as plain string errors; now return CodeInvalidArgument / CodeNotFound respectively.

Summary by CodeRabbit

  • Refactor

    • Upgraded internal RPC communication protocol to a modern framework for improved compatibility and reliability.
  • Chores

    • Updated core dependencies to the latest versions.
    • Streamlined development tooling configuration.

Review Change Stack

Plugin now exposes the generated kvV2connect.KvService handler instead
of the legacy net/rpc + goridge codec methods. RPC() returns
(path, http.Handler) so the rpc plugin's HTTP/2 mux mounts it directly
at /kv.v2.KvService/.

- plugin.go: RPC() any -> RPC() (string, http.Handler); import alias
  kvV2connect (matches proto go_package)
- rpc.go: 7 methods adapted to connect.Request/Response shapes; caller
  ctx now flows into the OTEL span (was context.Background()); empty
  Storage -> CodeInvalidArgument; missing storage -> CodeNotFound;
  internal errors wrap errors.E(op, err) with CodeInternal
- lookupStorage helper extracted to DRY the 7 identical empty/missing
  storage guards (was 14 lines x 7 = 98 lines of validation noise)
- tests: shared h2c Connect client; goridge codec dropped
- deps: api-go v6.0.0-beta.5, rpc/v6 v6.0.0-beta.4, connect v1.19.2,
  golang.org/x/net for h2c; goridge no longer imported in tests
- lint: drop dead settings.dupl block
Copilot AI review requested due to automatic review settings May 11, 2026 06:20
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 11, 2026

Warning

Rate limit exceeded

@rustatian has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 51 minutes and 18 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5b664d3b-5f06-4d63-8267-494a79037197

📥 Commits

Reviewing files that changed from the base of the PR and between fbd7028 and 167be9a.

⛔ Files ignored due to path filters (3)
  • go.sum is excluded by !**/*.sum
  • go.work.sum is excluded by !**/*.sum
  • tests/go.sum is excluded by !**/*.sum
📒 Files selected for processing (8)
  • go.mod
  • plugin.go
  • rpc.go
  • tests/configs/.rr-kv-api.yaml
  • tests/configs/.rr-kv-init.yaml
  • tests/go.mod
  • tests/kv_api_test.go
  • tests/kv_plugin_test.go
📝 Walkthrough

Walkthrough

This PR migrates the KV plugin's RPC interface from legacy net/rpc/Goridge to ConnectRPC v2 over HTTP/2. Dependencies are updated to api-go beta.5 with ConnectRPC support, all RPC handler methods are refactored to Connect-compatible signatures, the plugin entry point is rewired to expose HTTP handlers, tests are updated to use a ConnectRPC HTTP client, and linter configuration is cleaned up.

Changes

ConnectRPC v2 Migration

Layer / File(s) Summary
Dependency Updates
go.mod, tests/go.mod
github.com/roadrunner-server/api-go/v6 bumped to v6.0.0-beta.5; adds connectrpc.com/connect, google.golang.org/grpc v1.81.0, updates indirect gRPC/genproto deps; removes github.com/roadrunner-server/goridge/v4.
RPC Handler Refactoring
rpc.go
Seven RPC methods (Has, Set, MGet, MExpire, TTL, Delete, Clear) refactored from proto-style handlers to ConnectRPC handlers. Adds lookupStorage() helper for storage validation with Connect error codes; updates from() to compute TTL-based timeouts; each handler now opens tracing spans and wraps errors as Connect Internal/NotFound/InvalidArgument responses.
Plugin RPC Interface Wiring
plugin.go
RPC() method signature changed from func() any to func() (string, http.Handler). Imports net/http and kvV2connect package; constructs handler via kvV2connect.NewKvServiceHandler passing rpc implementation, storages, and tracer.
Test Client Migration
tests/kv_plugin_test.go
Adds newKvClient() helper to build ConnectRPC KV v2 client with HTTP/2 transport targeting http://127.0.0.1:6001. Updates kvSetTest and kvHasTest to call ConnectRPC client methods with connect.NewRequest instead of legacy net.Dialer + goridgeRpc codec.
Linter Configuration Cleanup
.golangci.yml
Removes dupl linter configuration (threshold: 100) from linters.settings.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

  • roadrunner-server/kv#107: Both PRs migrate KV RPC to v2/v6 API, updating handler signatures and plugin wiring for the v6 plugins framework.

Poem

🐰 From Goridge's old bridge to Connect's new way,
HTTP/2 flows where net/rpc held sway,
Seven handlers reborn with context and request,
Typed responses shine—this migration's the best!
Traces record errors, semantics take flight,
KV plugin's evolved to the future tonight. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description is comprehensive and well-structured. It clearly explains the changes across all files (plugin.go, rpc.go, tests, dependencies, and linter config) and explicitly documents breaking changes. However, the template requires specific sections that are not fully addressed: no explicit 'Reason for This PR' section with issue/reference, and the PR Checklist items are not checked or verified. Fill in the 'Reason for This PR' section with an issue number or clear explanation, and explicitly complete the PR Checklist with checkmarks and verification notes for all required items.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: migration to Connect-RPC with the new wire path. It is concise, specific, and clearly reflects the primary objective of the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/migrate-connect-rpc

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Apply Go 1.25 wg.Go() across the 4 test setups in kv_plugin_test.go,
dropping the parallel wg.Add(1) / defer wg.Done() pairs. No behavior
change; goroutine count remains the same.
@codecov
Copy link
Copy Markdown

codecov Bot commented May 11, 2026

Codecov Report

❌ Patch coverage is 44.68085% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.04%. Comparing base (8d37a5a) to head (167be9a).

Files with missing lines Patch % Lines
rpc.go 41.57% 43 Missing and 9 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #119       +/-   ##
===========================================
+ Coverage   34.83%   51.04%   +16.21%     
===========================================
  Files           3        3               
  Lines         267      239       -28     
===========================================
+ Hits           93      122       +29     
+ Misses        160       95       -65     
- Partials       14       22        +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- lookupStorage: switch to fmt.Errorf("%w: %s", errNoSuchStore, name)
  so callers can stderr.Is(err, errNoSuchStore) — the previous
  errors.Errorf("%s: %s", sentinel.Error(), name) embedded only the
  message text and broke the Is-chain.
- TTL handler: clamp negative time.Until(t) durations to 0 before
  emitting durationpb. An RFC3339 timestamp that has already elapsed
  was previously sent as a negative duration; clients would see
  nonsensical TTLs. (Carried-forward from PR #107 review comment;
  natural to fix while rewriting this block.)
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/kv_plugin_test.go (1)

340-359: ⚡ Quick win

Add regression tests for the new Connect error contract.

This only verifies the happy path, but this PR also changes the public error semantics in a breaking way. Please add cases for empty storage and unknown storage so CodeInvalidArgument and CodeNotFound stay pinned by tests.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/kv_plugin_test.go` around lines 340 - 359, Add regression tests
alongside kvSetTest/kvHasTest that exercise the new Connect error contract: call
newKvClient().Set and .Has with an empty Storage string (using
connect.NewRequest(&kvV2.KvRequest{Storage: "", Items: ...})) and assert the
returned error’s code is connect.CodeInvalidArgument, and call Set/Has with a
non-existent storage name like "unknown" and assert the returned error’s code is
connect.CodeNotFound; use connect.CodeOf(err) (or the project’s helper to
extract the Connect code) to compare against connect.CodeInvalidArgument and
connect.CodeNotFound so the tests pin the public error semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@rpc.go`:
- Around line 57-61: The storage handlers (e.g., st.Has) currently convert
context.Canceled and context.DeadlineExceeded into connect.CodeInternal; change
error mapping so context.Canceled -> connect.CodeCanceled and
context.DeadlineExceeded -> connect.CodeDeadlineExceeded by using a small helper
(e.g., mapContextErrorToConnectError(err) that returns the proper connect.Error)
instead of always wrapping as CodeInternal, and apply this helper in Has as well
as Set, MGet, MExpire, TTL, Delete, and Clear handlers to preserve
cancel/timeout semantics over the wire.

---

Nitpick comments:
In `@tests/kv_plugin_test.go`:
- Around line 340-359: Add regression tests alongside kvSetTest/kvHasTest that
exercise the new Connect error contract: call newKvClient().Set and .Has with an
empty Storage string (using connect.NewRequest(&kvV2.KvRequest{Storage: "",
Items: ...})) and assert the returned error’s code is
connect.CodeInvalidArgument, and call Set/Has with a non-existent storage name
like "unknown" and assert the returned error’s code is connect.CodeNotFound; use
connect.CodeOf(err) (or the project’s helper to extract the Connect code) to
compare against connect.CodeInvalidArgument and connect.CodeNotFound so the
tests pin the public error semantics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e6739478-efbf-4612-b3be-6754b789ad43

📥 Commits

Reviewing files that changed from the base of the PR and between 8d37a5a and fbd7028.

⛔ Files ignored due to path filters (2)
  • go.sum is excluded by !**/*.sum
  • tests/go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • .golangci.yml
  • go.mod
  • plugin.go
  • rpc.go
  • tests/go.mod
  • tests/kv_plugin_test.go
💤 Files with no reviewable changes (1)
  • .golangci.yml

Comment thread rpc.go
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the KV plugin’s RPC surface from the legacy goridge net/rpc codec to Connect-RPC over HTTP/2, aligning the plugin with the Connect-migrated rpc/v6 plugin and the new /kv.v2.KvService/ wire path.

Changes:

  • Switch plugin RPC registration to a Connect-generated HTTP handler and update rpc.go method signatures to the Connect handler interface.
  • Update KV integration tests to use connectrpc.com/connect with an h2c-capable HTTP/2 client instead of goridge/netrpc.
  • Bump api-go and related dependencies; clean up .golangci.yml by removing unused dupl settings.

Reviewed changes

Copilot reviewed 6 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugin.go Exposes the KV service as a Connect HTTP handler via the generated kvV2connect handler.
rpc.go Reworks KV RPC methods to Connect request/response types, propagating caller context and mapping errors to Connect codes.
tests/kv_plugin_test.go Migrates tests from goridge net/rpc to a Connect client using an h2c http2.Transport.
go.mod Updates core dependency on api-go to v6.0.0-beta.5 (but currently missing a required Connect dependency).
go.sum Updates sums for bumped/added transitive dependencies.
tests/go.mod Adds Connect + h2c dependencies and bumps api-go/rpc for the test module.
tests/go.sum Adds sums for new Connect/h2c-related dependencies used by tests.
.golangci.yml Removes a dead dupl settings block.
Comments suppressed due to low confidence (1)

go.mod:14

  • rpc.go now imports connectrpc.com/connect, but the root module go.mod / go.sum don’t include that module. In CI, this commonly fails with no required module provides package connectrpc.com/connect (especially when using -mod=readonly). Run go mod tidy (at the repo root) or add the missing requirement so the main module’s dependency graph includes Connect.
require (
	github.com/roadrunner-server/api-go/v6 v6.0.0-beta.5
	github.com/roadrunner-server/api-plugins/v6 v6.0.0-beta.2
	github.com/roadrunner-server/endure/v2 v2.6.2
	github.com/roadrunner-server/errors v1.5.0
	go.opentelemetry.io/otel/sdk v1.43.0
	google.golang.org/protobuf v1.36.11
)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/kv_plugin_test.go
Comment thread rpc.go
rustatian added 7 commits May 11, 2026 08:28
- rpc struct: {storages, *TracerProvider} -> {pl *Plugin, tracer
  trace.Tracer}. The tracer is now resolved once in Plugin.RPC() via
  p.tracer.Tracer(tracerName), so each handler entry skips OTEL's
  process-wide tracer-cache mutex/map lookup.
- storages access goes through r.pl.storages; future Plugin fields
  flow through without touching rpc.
- keysOf(items) helper replaces 4 identical 4-line key-extraction
  loops in Has/MGet/TTL/Delete. Each loop also avoided calling
  msg.GetItems() twice per iteration.
Pre-migration the spans started from context.Background(), so storage
errors were always real internal failures. Post-migration the request
ctx flows through; when a client cancels or hits its deadline mid-call,
storage backends return context.Canceled / context.DeadlineExceeded.
Wrapping those as CodeInternal told clients "server bug" when in fact
the truth is "your request was cancelled/timed out".

internalErr() inspects the storage error and picks the right code:
- context.Canceled         -> CodeCanceled
- context.DeadlineExceeded -> CodeDeadlineExceeded
- otherwise                -> CodeInternal

Applied across all 8 internal-error sites in Has/Set/MGet/MExpire/TTL/
Delete/Clear.

Also: tests/kv_plugin_test.go h2c Dialer gets a 30s Timeout so the
suite can't hang on a TCP connect stall if the rpc server is down.
Request body remains unbounded.

Resolves coderabbitai + Copilot threads on PR #119.
Per user preference: Connect handler bodies pull each field directly
from req.Msg, no local alias. Keeps the call site self-documenting.
New tests/kv_api_test.go (with a minimal in-memory config) verifies that
the same kv handler mounted on the rpc plugin serves both wire formats:

- TestKVHTTPApi: Connect-RPC over h2c, the protocol PHP clients use.
- TestKVGRPCApi: regular gRPC (google.golang.org/grpc) on the same port.

Both run a Set / Has / Delete / Has cycle and assert state changes
end-to-end. The shared startKvAPIContainer helper brings up rpc + kv +
memory + logger, so the suite stays Docker-free.
The kv handler is mounted once on the rpc plugin's mux but Connect-RPC
hands it three wire protocols out of the box. This commit adds one
test per protocol against the same in-memory storage, all running a
Set / Has / Delete / Has cycle:

- TestKVConnectAPI: Connect over h2c via connectrpc.com/connect.
- TestKVHTTPApi:    plain HTTP/1.1 POST with a protojson body and
  Content-Type: application/json. This is the shape PHP clients use
  (no Connect SDK; Guzzle/curl + json_encode).
- TestKVGRPCApi:    regular gRPC via google.golang.org/grpc + the
  gRPC-generated client. Used by PHP's gRPC extension.

Helper startKvAPIContainer brings up rpc + kv + memory + logger.
api-go v6.0.0-beta.6 (tagged off api PR #71) re-generates the
KvServiceHandler with WithIdempotency(IdempotencyNoSideEffects) on
Has / MGet / TTL. Connect now accepts HTTP GET on those, encoding the
request body in query params.

- Bump api-go: v6.0.0-beta.5 -> v6.0.0-beta.6 in root + tests modules.
- Replace the temporary TestKVHTTPGetRejected probe with a table-driven
  TestKVHTTPGetIdempotency that asserts:
    GET Has, MGet, TTL              -> 200 OK
    GET Set, MExpire, Delete, Clear -> 405 Method Not Allowed
  Provides regression coverage for the proto idempotency annotations.
schema.json's pattern allows only `driver` + optional `config` keys per
storage, with strict additionalProperties=false on each driver's config.
The kv-init/api configs had drifted:

- `default` / `in-memory` (memory): the memory driver's KvFromConfig
  ignores its argument outright, and the schema's `memory` branch
  requires `config: {}` (empty object). The configs had `interval: 60`
  — silently dropped at runtime, invalid by schema. Replace with `{}`.
- `boltdb-south` / `boltdb-africa` (boltdb): boltkv.Config exposes only
  `file`, `permissions`, `interval` via mapstructure. The configs had
  `dir: "."` (no such field) and `bucket: "rr"` (private, hardcoded to
  "default"). Both were silently swallowed; drop them.
- memcached entry was already valid; left alone.

No runtime behavior change. JSON Schema validators (IDE / CI tooling)
now accept the configs cleanly.

`.rr-kv-bolt-no-interval.yaml` and `.rr-kv-bolt-perms.yaml` already
matched the schema and are unchanged.
@rustatian rustatian self-assigned this May 11, 2026
@rustatian rustatian added the enhancement New feature or request label May 11, 2026
@rustatian rustatian merged commit 8bf6be4 into master May 11, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants